Skip to content

Split media store from event cache store.#5568

Merged
Hywan merged 5 commits intomainfrom
dragonfly1033/split_media_db
Sep 10, 2025
Merged

Split media store from event cache store.#5568
Hywan merged 5 commits intomainfrom
dragonfly1033/split_media_db

Conversation

@dragonfly1033
Copy link
Contributor

@dragonfly1033 dragonfly1033 commented Aug 22, 2025 •
edited
Loading

This PR is a start to the process of splitting the media store from the event cache store. #5410

It contains:

  • Split MediaStore trait from EventCacheStore.
  • Rename EventCacheStoreMedia to MediaStoreInner.
  • Move relevant tests into MediaStoreIntegrationTests.

This will be done over 3 PR's (reviewing 1, 2, 3 then merging 3 into 2 into 1).

A reminder comment for my own sanity:
This PR will not pass tests until after merging.

Current state of this PR:

Note, may also want to:

  • Re-organize file structure
  • Split/refactor benchmarks namely benchmarks/benches/event_cache.rs
  • Public API changes documented in changelogs (optional)

Signed-off-by: Shrey Patel shreyp@element.io

This was referenced Aug 22, 2025
Merged
Closed
Merged
@dragonfly1033 dragonfly1033 force-pushed the dragonfly1033/split_media_db branch from 561c12e to cef5dd3 Compare August 27, 2025 12:36
@dragonfly1033 dragonfly1033 marked this pull request as ready for review September 1, 2025 10:33
@dragonfly1033 dragonfly1033 requested a review from a team as a code owner September 1, 2025 10:33
@dragonfly1033 dragonfly1033 requested review from Hywan and andybalaam and removed request for a team and andybalaam September 1, 2025 10:33
@dragonfly1033 dragonfly1033 force-pushed the dragonfly1033/split_media_db branch 2 times, most recently from 7d3c4ad to 11fc140 Compare September 3, 2025 12:10
Hywan
Hywan previously approved these changes Sep 3, 2025
View reviewed changes
@dragonfly1033 dragonfly1033 force-pushed the dragonfly1033/split_media_db branch 2 times, most recently from 91ef9a2 to bec1d07 Compare September 3, 2025 14:25
@dragonfly1033 dragonfly1033 dismissed Hywan’s stale review September 3, 2025 14:28

Review was preliminary. New review will be needed aftere CI fixes and changelog entry as planned 🙂

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 3, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #5568 will not alter performance

Comparing dragonfly1033/split_media_db (8c1ca74) with main (b0c0e0e)

Summary

✅ 49 untouched benchmarks

@codecov
Copy link

codecov bot commented Sep 3, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 71.81303% with 199 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.31%. Comparing base (ce3b67f) to head (8c1ca74).
⚠️ Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk-sqlite/src/media_store.rs 70.80% 53 Missing and 27 partials ⚠️
...trix-sdk-base/src/media/store/integration_tests.rs 85.86% 25 Missing and 1 partial ⚠️
...es/matrix-sdk-base/src/media/store/memory_store.rs 71.26% 25 Missing ⚠️
crates/matrix-sdk-base/src/media/store/traits.rs 45.94% 20 Missing ⚠️
crates/matrix-sdk-base/src/media/store/mod.rs 69.23% 11 Missing and 1 partial ⚠️
crates/matrix-sdk/src/send_queue/upload.rs 50.00% 0 Missing and 8 partials ⚠️
crates/matrix-sdk/src/media.rs 22.22% 6 Missing and 1 partial ⚠️
crates/matrix-sdk-sqlite/src/error.rs 0.00% 5 Missing ⚠️
crates/matrix-sdk/src/client/builder/mod.rs 28.57% 1 Missing and 4 partials ⚠️
...s/matrix-sdk-base/src/media/store/media_service.rs 78.94% 2 Missing and 2 partials ⚠️
... and 2 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5568      +/-   ##
==========================================
- Coverage   88.89%   88.31%   -0.58%     
==========================================
  Files         349      353       +4     
  Lines       96546    96750     +204     
  Branches    96546    96750     +204     
==========================================
- Hits        85820    85448     -372     
- Misses       6688     7256     +568     
- Partials     4038     4046       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dragonfly1033 dragonfly1033 force-pushed the dragonfly1033/split_media_db branch 2 times, most recently from a46efed to ee05e50 Compare September 3, 2025 15:25
@Hywan
Copy link
Member

Hywan commented Sep 3, 2025

@dragonfly1033 Tell me if you need a last round of review.

@dragonfly1033
Copy link
Contributor Author

dragonfly1033 commented Sep 3, 2025

@dragonfly1033 Tell me if you need a last round of review.

Yes please! I've put all the unreviewed changes in the fixup commit. Just waiting for these benches to see if it's all good.

@dragonfly1033 dragonfly1033 requested a review from Hywan September 3, 2025 15:50
Hywan
Hywan reviewed Sep 9, 2025
View reviewed changes
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work! I've left a couple of questions.

@dragonfly1033 dragonfly1033 force-pushed the dragonfly1033/split_media_db branch 2 times, most recently from 9c2786d to c2e685c Compare September 9, 2025 13:40
@dragonfly1033 dragonfly1033 requested a review from Hywan September 9, 2025 14:14
Shrey Patel and others added 4 commits September 9, 2025 15:51
Split SqliteMediaStore from SqliteEventCacheStore.
1ebc619 
Split `MemoryMediaStore` from `MemoryStore`.
Split `IndexeddbMediaStore` from `IndexxeddbEventCacheStore`.
@dragonfly1033
Merge pull request #5571 from matrix-org/dragonfly1033/split_media_db_3
6b930c6 
Part 3 of Split media store from event cache store
Split MediaStore trait from EventCacheStore.
55bfa74 
Rename `EventCacheStoreMedia` to `MediaStoreInner`.
Move relevant tests into `MediaStoreIntegrationTests`.
@dragonfly1033
Merge pull request #5569 from matrix-org/dragonfly1033/split_media_db_2
8c1ca74 
Part 2 of Split media store from event cache store
@dragonfly1033 dragonfly1033 force-pushed the dragonfly1033/split_media_db branch from c2e685c to 8c1ca74 Compare September 9, 2025 14:52
@dragonfly1033 dragonfly1033 reopened this Sep 9, 2025
@Hywan Hywan merged commit 2afbdfa into main Sep 10, 2025
80 of 105 checks passed
@Hywan Hywan deleted the dragonfly1033/split_media_db branch September 10, 2025 10:03
@zecakeh zecakeh mentioned this pull request Sep 11, 2025
Merged
@Hywan Hywan mentioned this pull request Sep 12, 2025
Closed
@mgoldenberg mgoldenberg mentioned this pull request Sep 16, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Hywan Hywan Hywan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dragonfly1033 @Hywan